Skip to content

Conversation

BorntraegerMarc
Copy link
Contributor

Signed-off-by: BorntraegerMarc [email protected]

@BorntraegerMarc
Copy link
Contributor Author

@wzrdtales Created new PR to comply to all project requirements. Tests look OK to you?

@wzrdtales
Copy link
Member

Looks good so far, I had a look over this again now, did you tested this for functionality?

https://github.com/BorntraegerMarc/mongodb/blob/cfe208c5805ee244560b9343490b9e5e31eca323/index.js#L306

As you call this callback here, the river would get closed before it gets to the user, this would need to keep open, which also needs some information for the user that they need to close the connection when they request an instance in that way.

…stance method is called

Signed-off-by: BorntraegerMarc <[email protected]>
@BorntraegerMarc
Copy link
Contributor Author

@wzrdtales good catch! I've added an if statement there. Looks good now?

@wzrdtales
Copy link
Member

I would rather call prCB directly instead of introducing a string heavy comparison which was already evaluated once before.

index.js Outdated
db.collection(collection)[command](options.query, options.update, options.options, callbackFunction);
break;
case 'getDbInstance':
prCB(null, db); // When the user wants to get the DB instance we need to retrun the promise callback, so the DB connection is not automatically closed
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wzrdtales like this you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I typo slipped in retrun instead of return :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wzrdtales Good catch 😄 Corrected it

Signed-off-by: BorntraegerMarc <[email protected]>
@wzrdtales
Copy link
Member

Looks good, merging at this point.

@wzrdtales wzrdtales merged commit 835b64b into db-migrate:master Jan 8, 2018
@BorntraegerMarc
Copy link
Contributor Author

Thanks @wzrdtales ! Could you release a new npm version so I can test out the functionality? 😄

@wzrdtales
Copy link
Member

later this evening yes (I'm gmt+1 though)

@wzrdtales
Copy link
Member

Was just published as

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants